[test] Refactor integration tests to directly write/read ptrace.Traces
#7812
Conversation
Signed-off-by: Manik Mehta <[email protected]>
Signed-off-by: Manik Mehta <[email protected]>
|
Currently fixing the ES tests |
Signed-off-by: Manik Mehta <[email protected]>
Signed-off-by: Manik Mehta <[email protected]>
Signed-off-by: Manik Mehta <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7812 +/- ##
==========================================
+ Coverage 95.49% 95.50% +0.01%
==========================================
Files 305 305
Lines 16174 16174
==========================================
+ Hits 15445 15447 +2
+ Misses 571 570 -1
+ Partials 158 157 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Metrics Comparison SummaryTotal changes across all snapshots: 0 Detailed changes per snapshotsummary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 0
summary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 0
|
Signed-off-by: Manik Mehta <[email protected]>
|
Majority of tests are passing, except kafka. Working on it!. @yurishkuro Please can you review it! |
Signed-off-by: Manik Mehta <[email protected]>
|
Want to ask about kafka, do kafka also assigns one span per resource span? Like ES/OS? Also I can't understand the encoding part, how is kafka tested? i mean to ask how it is different from other storage tests? |
| return 0 | ||
| } | ||
|
|
||
| func checkSize(t *testing.T, expected *model.Trace, actual *model.Trace) { |
There was a problem hiding this comment.
no need to check size, ptracetest.CompareTraces checks for us
| ptracetest.IgnoreSpansOrder(), | ||
| } | ||
| if err := ptracetest.CompareTraces(expected, actual, options...); err != nil { | ||
| t.Logf("Actual trace and expected traces are not equal: %v", err) |
There was a problem hiding this comment.
no need of pretty diff, CompareTraces gives first point of difference as error
There was a problem hiding this comment.
have you tried comparing JSON strigns instead? I think it's nice to get a full dump of diffs, not the first breaking point.
Example:
import (
"github.com/hexops/gotextdiff"
"github.com/hexops/gotextdiff/myers"
"github.com/hexops/gotextdiff/span"
)
func DiffStrings(want, got string) string {
edits := myers.ComputeEdits(span.URIFromPath("want.json"), want, got)
diff := gotextdiff.ToUnified("want.json", "got.json", want, edits)
return fmt.Sprint(diff)
}
| return bytes.Compare(aAttrs[:], bAttrs[:]) | ||
| } | ||
|
|
||
| func compareTimestamps(a, b pcommon.Timestamp) int { |
There was a problem hiding this comment.
we could directly subtract the timestamps but that would require unnecessary and risky conversion of uint64 to int
| t.Log(err) | ||
| return false | ||
| } | ||
| if len(expected) != len(traces) { |
There was a problem hiding this comment.
We don't know the number of traces which reader will give in one slice, so this check becomes useless
|
Another problem is that for |
Signed-off-by: Manik Mehta <[email protected]>
Are you generating more verbose traces which exceeded the message limit? There should be settings in both Kafka broker and exporter about max message size. Alternatively we can use a smaller batch in the collector config, there's no reason to send all 1000 spans as a single message |
|
|
I remember there was an outstanding ticket in upstream OTEL contrib about Kafka exporter not respecting max batch size / message size, i.e. the batch processor can make the message bigger by aggregating multiple |
yes, the issue is still there! |
Signed-off-by: Manik Mehta <[email protected]>
| _ io.Closer = (*traceWriter)(nil) | ||
|
|
||
| MaxChunkSize = 35 // max chunk size otel kafka export can handle safely. | ||
| MaxChunkSize = 5 // max chunk size otel kafka export can handle safely. |
There was a problem hiding this comment.
reducing to this has passed the tests
|
@yurishkuro Please review |
| loadAndParseJSONPB(t, fileName, &trace) | ||
| return &trace | ||
| // getNormalisedTraces normalise traces and assign one resource span to one span | ||
| func getNormalisedTraces(td ptrace.Traces) ptrace.Traces { |
There was a problem hiding this comment.
use usually use American spelling
| func getNormalisedTraces(td ptrace.Traces) ptrace.Traces { | |
| func getNormalizedTraces(td ptrace.Traces) ptrace.Traces { |
I am not clear why this function is needed.
There was a problem hiding this comment.
some storages like Elasticsearch, Clickhouse takes traces and save spans with scope and resource embedded in them so reader returns one resource span per span that means spans having same resource are not under same resource span so in integration tests, we need to normalize the fixtures to follow this. Some fixtures regroup the spans with same resource into same resource span which fails the tests
| ptracetest.IgnoreSpansOrder(), | ||
| } | ||
| if err := ptracetest.CompareTraces(expected, actual, options...); err != nil { | ||
| t.Logf("Actual trace and expected traces are not equal: %v", err) |
There was a problem hiding this comment.
have you tried comparing JSON strigns instead? I think it's nice to get a full dump of diffs, not the first breaking point.
Example:
import (
"github.com/hexops/gotextdiff"
"github.com/hexops/gotextdiff/myers"
"github.com/hexops/gotextdiff/span"
)
func DiffStrings(want, got string) string {
edits := myers.ComputeEdits(span.URIFromPath("want.json"), want, got)
diff := gotextdiff.ToUnified("want.json", "got.json", want, edits)
return fmt.Sprint(diff)
}
|
#7812 (comment) The problem in comparing strings are attributes. There is no way to sort attributes. I tried unmarshalling traces and then comparing bytes but attributes order became the problem. |
Signed-off-by: Manik Mehta <[email protected]>
Signed-off-by: Manik Mehta <[email protected]>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
@yurishkuro can you please see this comment #7812 (comment) |
yurishkuro
added
the
changelog:ci
| // from the current UTC date, while preserving the original time | ||
| // This is required in integration tests because the fixtures have | ||
| // hardcoded start time and other time stamps and we need to make | ||
| // recent to fetch from the reader. |
There was a problem hiding this comment.
we need to make recent to fetch from the reader
Did similar enrichment exist with v1 model fixtures?
There was a problem hiding this comment.
yes, here it is:
| require.NoError(t, err) | ||
| } | ||
|
|
||
| func mergeTraces(traceIter [][]ptrace.Traces) ptrace.Traces { |
There was a problem hiding this comment.
please use MergeTraces from ./internal/jptrace/aggregator.go
There was a problem hiding this comment.
but that merge traces aims to solve a different problem. It copies all resource spans from source to destination and here we require to merge all the traces from slice
| expected := expectedTracesPerTestCase[i] | ||
| actual := s.findTracesByQuery(t, queryTestCase.Query.ToTraceQueryParams(), expected) | ||
| CompareSliceOfTraces(t, expected, actual) | ||
| expectedTrace := mergeTraces([][]ptrace.Traces{expected}) |
There was a problem hiding this comment.
mergeTraces performs unconditional merge, i.e. if you have more than one trace they will all get merged into a single ptrace.Traces object. Is that what you need here? Is it not possible that the query tests return more than one trace?
There was a problem hiding this comment.
Yes because its easier to compare traces then. Because the backends might not send traces in the exact expected way. For example it may be possible that we are expecting all traces to be sent in first slice of result but backends may send them in different slices. Although what I have understood is that in a single slice we expect traces of same trace id but different in resource or scope. If my assumption is correct still not all backends support scope. Even if we leave this, we have normalization for backends which makes comparisions of slices of traces even more complex. So I thought of merging them in a single trace and the simply comparing.
| } | ||
| if spanCount(expected) != spanCount(traces) { | ||
| t.Logf("Excepting certain number of spans: expected: %d, actual: %d", spanCount(expected), spanCount(traces)) | ||
| traces = mergeTraces(traceSlice) |
There was a problem hiding this comment.
similar question - we might be merging multiple traces into one, is that what you want?
Signed-off-by: Manik Mehta <[email protected]>
|
I noticed checking the scope of issue #7050 that it also mentions: "Incrementally swap test fixtures to be stored as OTLP JSON instead of v1 model JSON". I am interested in picking up that part of the task (converting the fixtures/traces/*.json files to OTLP). Since you are working on the integration tests, would you mind if I worked on the fixture conversion in parallel? Or did you plan to cover that as well? Also, My PR #7761 which attempted some of the integration test cleanup is currently under review, but your changes here seem to cover the logic side more comprehensively with direct ptrace comparison. so should i close it? |
You can start swapping fixtures after this PR is merged as before that it is not quite possible. Swapping the first fixture would require some more changes and after that you can work on it. I myself was thinking for a help from the community as the number of fixtures are high and it will take large time to refactor them all by a single contributor. I will update in the issue thread. |
3 participants
Which problem is this PR solving?
Description of the changes
ptrace.Tracesdirectly.How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test